Size Change: +16 B (0%)

Total Size: 1.07 MB

_{compressed-size-action}

This doesn't work well when you apply the isShady variation in CardHeader or CardFooter... (You can try in storybook)

Can you elaborate on that? As far as I can tell, the box-shadow/border is removed intentionally and entirely when is-shady is applied:

@jasmussen no it's not removed intentionally, you can try reverting your change and see the result (it's specially not great for CardBody and Footer IMO)

Of course yes, thank you, I see it now:

The alternative I can think of is to change the border-radius value so that it renders as 2px despite being outset. We've done that in some of the SCSS files using border-radius: $radius-block-ui - $border-width; with a comment above it. Would something similar work here?

Edit: I have a separate question on whether we should have an "is-shady" property for the card component at all, but that's unrelated to your point.

Edit: I have a separate question on whether we should have an "is-shady" property for the card component at all, but that's unrelated to your point.

That is something I was wondering as well. Maybe we could drop support for that entirely (especially for body) but it'd still be great if the border is shown around the "shady" header as this is more common. Components folks should have a better opinion here @ciampo

The reason why the original approach is not working is that the inset box-shadow gets rendered underneath the other child elements (CardHeader, CardBody, CardFooter, ...).
When CardHeader is in its normal state, its background is transparent, and therefore it's possible to see-through it. When the isShady prop is toggled, CardHeader's background becomes a solid color (light gray), and therefore the inset box-shadow is hidden under it.

One possible solution is to use a pseudo-element (via ::before or ::after) to render the box-shadow on top of the card's content (and use pointer-events: none to have all mouse events pass through this new layer).

diff --git a/packages/components/src/card/styles.js b/packages/components/src/card/styles.js
index 70280ebc72..c8ce8f32b2 100644
--- a/packages/components/src/card/styles.js
+++ b/packages/components/src/card/styles.js
@@ -9,8 +9,20 @@ import { css } from '@emotion/react';
 import { COLORS, CONFIG } from '../utils';
 
 export const Card = css`
-	box-shadow: inset 0 0 0 1px ${ CONFIG.surfaceBorderColor };
 	outline: none;
+
+	&::before {
+		content: '';
+
+		box-shadow: inset 0 0 0 1px ${ CONFIG.surfaceBorderColor };
+		position: absolute;
+		top: 0;
+		right: 0;
+		bottom: 0;
+		left: 0;
+
+		pointer-events: none;
+	}
 `;
 
 export const Header = css`
@@ -77,7 +89,9 @@ export const borderColor = css`
 `;
 
 export const boxShadowless = css`
-	box-shadow: none;
+	&::before {
+		content: none;
+	}
 `;
 
 export const borderless = css`

From a quick look, this seems to apply the changes that @jasmussen was looking for, without the drawbacks of the original approach. Although we'd need to investigate this a bit better to make sure we're not introducing a breaking change.

I have a separate question on whether we should have an "is-shady" property for the card component at all, but that's unrelated to your point.

That is something I was wondering as well. Maybe we could drop support for that entirely (especially for body) but it'd still be great if the border is shown around the "shady" header as this is more common. Components folks should have a better opinion here @ciampo

I'm not sure about the historical reasons for this prop to exists. Card is one of the oldest and most used components, and it's hard to know if removing the 'isShady` prop is going to cause much disruption. I guess at most we could look into marking it as deprecated and remove it from docs, while still supporting it in code (and eventually remove it once enough time has passed from the deprecation notice)?

I'm not sure about the historical reasons for this prop to exists. Card is one of the oldest and most used components

Actually Card is not that old in the history of Gutenberg and is only used twice in Core's codebase.

I'm not sure about the historical reasons for this prop to exists. Card is one of the oldest and most used components

Actually Card is not that old in the history of Gutenberg and is only used twice in Core's codebase.

Oh ok, thank you for sharing the knowledge! I remember that it is used in WordPress.com and WooCommerce, although I don't know to which extent.

I wonder what an actionable next step on this one is. I wouldn't call it urgent, but it seems like a nice little microcosm of adopting and adapting the components, and therefore a good exercise:

• Where's the card used at the moment?
• What is it used for, and does it accomplish that?

While I imagine a bordered card can be useful, if we find that it isn't, it might be worth simply removing that prop entirely. Just as an example, the ItemGroup is bordered, as are each item inside, and they always will be. This one probably doesn't need any prop to tell it to be bordered. Are we looking at something similar for the card?

I think a good approach here could be to treat each question separately:

• We could merge this PR with the improvements to the border (I pushed the changes that I had previously suggested, updated tests and rebased)
• We could then investigate / discuss in a separate issue the component's usage and the need for:
  • the isShady prop
  • borders (and, consequently, the isBorderless prop)

What do folks think?

Thanks for sheparding this along!

I do wonder if the pseudo element is overding it, though, considering it's just a border. It's not a strong opinion I have — but might there be side effects we don't want? Did we look at the usage of the component and see whether a "bordered" version was even necessary?

I do wonder if the pseudo element is overding it, though, considering it's just a border. It's not a strong opinion I have — but might there be side effects we don't want?

The only side - effect I can think of is if someone was already relying on the ::before pseudo element to achieve any other feature. If you prefer, we can also introduce one additional element (instead of using ::before) and apply the same styles.

Did we look at the usage of the component and see whether a "bordered" version was even necessary?

I has a quick look around the repo to see how the component is used:

• in the Flyout component
• in the Navigator story
• in the Preferences modal — explicitly border-less
• In the Edit navigation UI
• In the Global Styles sidebar for the preview (which is the screenshot in this PR's description) and wrapping all screens

On top of these usage, I know for sure that this component is currently used in WordPress.com (I believe for the cards used to display the "in-editor" welcome tour) and in some WooCommerce dashboard. There may be more usages that we're not aware of.

If we need just a "empty box", we could use the Surface component instead.

IMO, when I think of a Card component, I can see the need for a border. It's also worth noting that the Card component has a border by default, unless explicitly disabled with the isBorderless prop.

Good thoughts. I recall @jameskoster mentioning a "Card" component back in the day, if you have time to look, what are your thoughts? Is a "bordered" property a good aspect of a card? What do you see the card accomplishing? Thanks all.

The border property is useful in situations where a card sits on a field that shares the background color.

Probably a dumb question, but is there a reason we're using a box-shadow here instead of a simple border?

Probably a dumb question, but is there a reason we're using a box-shadow here instead of a simple border?

The primary reason for me is to keep a consistent and predictable 2px border radius, which works because the shadow can overlay inwards. Of course that can be worked around. The other reason is to make the border color semitransparent, so it picks up color from the background of the card, as shown in the 2nd picture of the PR. I don't have strong opinions in either direction, so long as we land on the right radius.

The primary reason for me is to keep a consistent and predictable 2px border radius

border always respects border-radius, no? 🤔

The other reason is to make the border color semitransparent, so it picks up color from the background of the card, as shown in the 2nd picture of the PR

That one seems a bit situational. I can see how it works elegantly here, but it might be less desirable in other situations. If anything this feels like it should be a separate property 😬

The border radius always works, yes, but if I apply border-radius: 2px; to the card, the border will be outset and come across as a 3px radius. The fix is something like border-radius: $radius-block-ui - $border-width;, which is fine.

The overlaid border is situational, yes, but I've come to be very careful with adding borders as they so quickly end up adding noise, conflicting with input fields and focus styles. And anything we can do to reduce borders, including have them blend better with background colors, is usually a win.

But it's not a strong opionin. I like the simplicity of border. We just want to make sure and account for what offset it might add, if any, if a bordered card is to stack next to a non-bordered card, so there isn't a pixel shift.

The border radius always works, yes, but if I apply border-radius: 2px; to the card, the border will be outset and come across as a 3px radius.

When using a shadow, yes. I'm suggesting to use border rather than a shadow. That way the radius is always respected.

I've come to be very careful with adding borders

I 100% agree with this sentiment, but we can't always know the circumstances in which Card will be used. A border is a pretty standard feature of a Card component evidenced by its usage on both WordPress.com and WooCommerce.

We just want to make sure and account for what offset it might add

We could potentially offset the padding based on the border width? 🤔

Here's a codepen.

@griffbrad do you remember if there was any technical reason why the Card component used box-shadow to render a border?

@griffbrad do you remember if there was any technical reason why the Card component used box-shadow to render a border?

I don’t believe there is a technical reason, no. @ItsJonQ’s original Card component didn’t take this approach itself. I do think @jasmussen’s rationale above, though (“to make the border color semitransparent, so it picks up color from the background of the card”) aligns with design feedback I’ve seen on other work recently.

Hey @jasmussen , what should the next steps be for this PR? Should we go ahead and test/merge these changes, or should we close it?

I'm a little nervous about using the pseudo selector approach for this, it feels like it's adding some complexity to something that should probably be simple.

I think it's fine to shelve/close/pause this PR for now. Even as an exploration, it has been valuable in surfacing some of the needs of the system, compared to what the components offer. For example I remain unconvinced that we need a cardheader and card footer at all — or even just that we don't want border separators between them. And if we were to decide in that way, it might offer us a different path forward to our goal that simplifies things.

As is, and looking purely as Gutenberg as the consumer, the Card appears to have been designed in isolation without a clear problem to solve. That's not a critique of the work, but rather a challenge to our assumptions about which components we should have, and how they should be used.

I'm a little nervous about using the pseudo selector approach for this, it feels like it's adding some complexity to something that should probably be simple.

This makes sense

For example I remain unconvinced that we need a cardheader and card footer at all — or even just that we don't want border separators between them. [...] As is, and looking purely as Gutenberg as the consumer, the Card appears to have been designed in isolation without a clear problem to solve.

I definitely share a similar opinion too — not exactly sure why we have card headers / footers / body (although I do think that it's nice to have the possibility to show a border).

From what I understand, the components package was initially created by putting together a few "shared" components across Gutenberg. The package lacked an overarching design system, and its components were mostly built out of necessity — which ultimately means that components are not always "consistent" and "coeherent" in terms of design language, public APIs, and also implementation.

That's not a critique of the work, but rather a challenge to our assumptions about which components we should have, and how they should be used.

Absolutely. The challenge now is to incrementally improve this package without introducing breaking changes — and your feedback has definitely helped a lot already!

I'm going to close this PR for now, we can always re-open it in case